Skip to content

Conversation

npmccallum
Copy link
Contributor

This is an expansion of #143773 for the Cow From conversions.

r? @oli-obk

@rustbot
Copy link
Collaborator

rustbot commented Sep 1, 2025

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 1, 2025
#[unstable(feature = "bstr", issue = "134915")]
impl<'a> From<ByteString> for Cow<'a, ByteStr> {
#[rustc_const_unstable(feature = "const_from", issue = "143773")]
impl<'a> const From<ByteString> for Cow<'a, ByteStr> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have the equivalent for String? And is ByteString const constructible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk Yes, the equivalent for String is in this same patch.

ByteString is const constructible:

#[repr(transparent)]
pub struct ByteString(pub Vec<u8>);

Both Vec and String are const constructible:

https://doc.rust-lang.org/std/vec/struct.Vec.html#method.new
https://doc.rust-lang.org/std/string/struct.String.html#method.new

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh lol i stopped scrolling and didnt see the other files

@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 1, 2025

📌 Commit 2784327 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2025

@bors rollup

fmease added a commit to fmease/rust that referenced this pull request Sep 1, 2025
alloc: make Cow From impls const

This is an expansion of rust-lang#143773 for the `Cow` `From` conversions.

r? `@oli-obk`
bors added a commit that referenced this pull request Sep 2, 2025
Rollup of 10 pull requests

Successful merges:

 - #144066 (stabilize c-style varargs for sysv64, win64, efiapi, aapcs)
 - #145783 (add span to struct pattern rest (..))
 - #145961 (resolve: Avoid a regression from splitting prelude into two scopes)
 - #145962 (Ensure we emit an allocator shim when only some crate types need one)
 - #146064 (Add compiler error when trying to use concat metavar expr in repetitions)
 - #146067 (alloc: make Cow From impls const)
 - #146070 (rustdoc-search: skip loading unneeded fnData)
 - #146089 (fix a constness ordering bug in rustfmt)
 - #146094 (Make `Parser::parse_for_head` public for rustfmt usage)
 - #146102 (Remove dead code stemming from an old effects desugaring)

r? `@ghost`
`@rustbot` modify labels: rollup
@clarfonthey
Copy link
Contributor

clarfonthey commented Sep 2, 2025

Does anyone read the tracking issues any more?

Whatever. I explicitly had this as part of #144289 and was not including it because the other reviewer, @tgross35, expressed apprehension about adding heap-allocated trait impls at the end. Part of this PR exists as #145279, which you would find out if you read the comments on the tracking issue you linked.

jhpratt added a commit to jhpratt/rust that referenced this pull request Sep 2, 2025
alloc: make Cow From impls const

This is an expansion of rust-lang#143773 for the `Cow` `From` conversions.

r? `@oli-obk`
bors added a commit that referenced this pull request Sep 2, 2025
Rollup of 9 pull requests

Successful merges:

 - #145783 (add span to struct pattern rest (..))
 - #145961 (resolve: Avoid a regression from splitting prelude into two scopes)
 - #145962 (Ensure we emit an allocator shim when only some crate types need one)
 - #146064 (Add compiler error when trying to use concat metavar expr in repetitions)
 - #146067 (alloc: make Cow From impls const)
 - #146070 (rustdoc-search: skip loading unneeded fnData)
 - #146089 (fix a constness ordering bug in rustfmt)
 - #146094 (Make `Parser::parse_for_head` public for rustfmt usage)
 - #146102 (Remove dead code stemming from an old effects desugaring)

r? `@ghost`
`@rustbot` modify labels: rollup
@oli-obk
Copy link
Contributor

oli-obk commented Sep 2, 2025

@bors r-

@oli-obk oli-obk closed this Sep 2, 2025
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 2, 2025
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 2, 2025
@npmccallum npmccallum deleted the cow branch September 3, 2025 00:27
@npmccallum
Copy link
Contributor Author

@oli-obk @clarfonthey @tgross35 I don't think this PR should have been rejected. Cow is often used to bridge cases where allocation may or may not occur. When it doesn't, we don't want to force non-const on the developer. With these const From traits, you can effectively use Cow in generic contents where the conditionality of the const depends on the implementor. For example:

const trait Textify<'a> {
    type TextType: [const] Into<Cow<'a, str>>;
    
    fn text(&self) -> Self::TextType;
}

In this case, the implementor can choose const or not based upon the implementation of text().

@npmccallum
Copy link
Contributor Author

Another simpler case is where conditional compilation is used. Here's a somewhat contrived example:

#[cfg(feature = "alloc")]
type Text<'a> = alloc::borrow::Cow<'a, str>;

#[cfg(not(feature = "alloc"))]
type Text<'a> = &'a str;

const fn decode<'a>(bytes: &'a [u8]) -> Text<'a> {
    core::str::from_utf8(bytes).unwrap().into() // Cow needs a const From impl here.
}

The key point is that without the impl const From for Cow, you'd have to conditionally compile at every site where Text is constructed.

@tgross35
Copy link
Contributor

These implementations will absolutely wind up const eventually; there isn't any issue there. The problem is that const traits are not yet through RFC and stable, and we want to keep the surface area reasonably minimal until they are stable. These are indeed trivial implementations, but alloc is an easy cutoff point; anything alloc/std is going to be a "nice to have" implementation, rather than something fundamentally blocking the ability to experiment with const traits.

We do not have an official policy here (I've been meaning to write a proposal) so it's possible the others feel differently, but I would still prefer to wait until const traits are stable for implementations like this. There isn't any rush.

See also #t-compiler/const-eval > SystemTime regression #146228

@npmccallum
Copy link
Contributor Author

These implementations will absolutely wind up const eventually; there isn't any issue there. The problem is that const traits are not yet through RFC and stable, and we want to keep the surface area reasonably minimal until they are stable. These are indeed trivial implementations, but alloc is an easy cutoff point; anything alloc/std is going to be a "nice to have" implementation, rather than something fundamentally blocking the ability to experiment with const traits.

We do not have an official policy here (I've been meaning to write a proposal) so it's possible the others feel differently, but I would still prefer to wait until const traits are stable for implementations like this. There isn't any rush.

See also #t-compiler/const-eval > SystemTime regression #146228

➜  rust git:(master) $ egrep -R '(const_convert|const_default)' library/alloc
library/alloc/src/lib.rs:#![feature(const_convert)]
library/alloc/src/lib.rs:#![feature(const_default)]
library/alloc/src/borrow.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
library/alloc/src/borrow.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
library/alloc/src/borrow.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
library/alloc/src/vec/mod.rs:#[rustc_const_unstable(feature = "const_default", issue = "143894")]
library/alloc/src/string.rs:#[rustc_const_unstable(feature = "const_default", issue = "143894")]
library/alloc/src/collections/mod.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
library/alloc/src/collections/mod.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]

@tgross35 There are already const implementations in alloc. Drawing the line at this PR feels rather arbitrary.

@tgross35
Copy link
Contributor

In retrospect I should have requested them dropped from #145279. I don't think there is any need to remove them at this time, given they aren't particularly invasive, but I don't think it should serve as a precedent for adding more.

Again, others may feel differently and we don't yet have a policy. And the incremental change isn't much. But I really don't think anybody's const experimentation is blocked on wrappers around Cow::Borrow/Cow::Owned, both of which already work perfectly fine in const today.

@npmccallum
Copy link
Contributor Author

@tgross35 But I really don't think anybody's const experimentation...

Mine is. That's why I wrote the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants